Doh, my attempt at testing is probably failing due to the connection caching. Will have to revisit.

Codecov Report

Merging #3149 into master will decrease coverage by 44.79%.
The diff coverage is 38.88%.

@@            Coverage Diff             @@
##           master    #3149      +/-   ##
==========================================
- Coverage   89.47%   44.68%   -44.8%     
==========================================
  Files         248      248              
  Lines       32703    32717      +14     
==========================================
- Hits        29262    14619   -14643     
- Misses       3441    18098   +14657

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ca3225e...3a48742. Read the comment docs.

Codecov Report

Merging #3149 into master will decrease coverage by 4.14%.
The diff coverage is 96.77%.

@@            Coverage Diff             @@
##           master    #3149      +/-   ##
==========================================
- Coverage   90.71%   86.56%   -4.15%     
==========================================
  Files         249      249              
  Lines       32723    32743      +20     
==========================================
- Hits        29684    28345    -1339     
- Misses       3039     4398    +1359

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 81bfe17...93c8651. Read the comment docs.

Hmph, with the latest iteration (3a48742), I'm stumped on why test_ssh_manager_close fails with the addition of test_ssh_custom_identity_file. Seems like it'd have to be some sort of order/state dependence, but I haven't had luck figuring out what's going on.

Could it be due to all already existing control paths we established in other tests?

Could it be due to all already existing control paths we established in other tests?

Right, that was along the lines of what I was thinking when I wrote "some sort of order/state dependence". The problem is that, even if the new test were using the same socket (it's not) and if it weren't closing it (it is), I don't see how test_ssh_manager_close would fail the way that it does---saying that the path for the datalad-test socket doesn't exist.

There's Travis-specific setup for these tests, so I've been restricting my local debugging to test_ssh_custom_identity_file (with an adjustment to its ifile variable). I'm next going to try to get all the tests in test_sshconnector.py running locally to see if I can trigger the issue and step through the failing test_ssh_manager_close.

OK, so it looks like the issue is that, after patching os.environs DATALAD_SSH_IDENTITYFILE, the value leaks into the other tests because get_connection was only doing a cfg.reload(), not cfg.reload(force=True).

So I guess the main question is who should be responsible for calling reload(force=True), SSHManager or its creators? If the answer is SSHManager, then there is the follow-up question of where SSHManager should do it, get_connection or just at initialization (__init__ or assure_initialized)?

I'm leaning towards making SSHManager do it in assure_initialized because that function already imports ConfigManager (not datalad.cfg), so it essentially does a reload(force=True) and we can piggyback on it without an added expense. I'll do that unless someone argues otherwise.

I'm leaning towards making SSHManager do it in assure_initialized because [...]

Done with latest push.

4:  c89249d29 = 1:  9cf3e0f2d TST+BF: sshconnector: Fix stale socket path construction
1:  f0377e599 = 2:  e9009eced ENH: ssh: Add identity_file parameter to get_connection_hash()
2:  30e7e18af = 3:  dc3226f5e ENH: SSHConnection: Support custom identity files
3:  8bf6a2ce9 ! 4:  93c86517b ENH: ssh: Support configuring an identity file
    @@ -32,21 +32,44 @@
      diff --git a/datalad/support/sshconnector.py b/datalad/support/sshconnector.py
      --- a/datalad/support/sshconnector.py
      +++ b/datalad/support/sshconnector.py
    +@@
    + class SSHManager(object):
    +     """Keeps ssh connections to share. Serves singleton representation
    +     per connection.
    ++
    ++    Note: If a connection should use a custom identity file via
    ++    `datalad.ssh.identityfile`, that value should be set before this class is
    ++    initialized.
    +     """
    + 
    +     def __init__(self):
    +@@
    +         # handling of socket_dir, so we do not define them here explicitly
    +         # to an empty list to fail if logic is violated
    +         self._prev_connections = None
    ++        self._identity_file = None
    +         # and no explicit initialization in the constructor
    +         # self.assure_initialized()
    + 
    +@@
    +         cfg = ConfigManager()
    +         self._socket_dir = opj(cfg.obtain('datalad.locations.cache'),
    +                                'sockets')
    ++        self._identity_file = cfg.get('datalad.ssh.identityfile')
    +         assure_dir(self._socket_dir)
    +         try:
    +             chmod(self._socket_dir, 0o700)
     @@
                  raise ValueError("Unsupported SSH URL: '{0}', use "
                                   "ssh://host/path or host:path syntax".format(url))
      
    -+        from datalad import cfg
    -+        # Reload because Python callers may have set environment variable after
    -+        # importing datalad. `force` needs to be true so that temporary
    -+        # environmental variables don't linger.
    -+        cfg.reload(force=True)
    ++        # Ensure self._identityfile has been set, if configured.
    ++        self.assure_initialized()
     +
    -+        identity_file = cfg.get('datalad.ssh.identityfile')
              conhash = get_connection_hash(
                  sshri.hostname,
                  port=sshri.port,
    -+            identity_file=identity_file or "",
    ++            identity_file=self._identity_file or "",
                  username=sshri.username)
              # determine control master:
              ctrl_path = "%s/%s" % (self.socket_dir, conhash)
    @@ -55,7 +78,8 @@
                  return self._connections[ctrl_path]
              else:
     -            c = SSHConnection(ctrl_path, sshri)
    -+            c = SSHConnection(ctrl_path, sshri, identity_file=identity_file)
    ++            c = SSHConnection(ctrl_path, sshri,
    ++                              identity_file=self._identity_file)
                  self._connections[ctrl_path] = c
                  return c
      

I'm leaning towards making SSHManager do it in assure_initialized

Eh after hooking up this latest approach to ReproMan, I'm not sure I made the right decision (I should have tested earlier). I was hoping that using assure_initialized would mean that the config reading would be delayed until datalad.ssh_managers first get_connection call (the only other spot where SSHManager calls assure_initialized is _socket_dir). But GitRepo.__init__() also calls assure_initialized. This means that if any external python callers want to set the environment variable to override this, they need to do it before an GitRepo initialization/use (e.g., using Dataset.id), not just before anything that might call get_connection.

So the options I see are

• Call config.reload(force=True) for each get_connection() call. That's the expense that the above approach was trying to avoid, but I'm not sure how bad it is in practice.

• Go back to the first iteration of this PR and just check os.environ, dropping the common_cfg entry.

• Do nothing. It doesn't seem that bad to say the python callers just need to set up the environment early, but depending on how the program is structured and the task, it might be difficult or impossible for it to set the environment variables before it initializes a GitRepo. And I don't believe there is currently an easy way for the caller to explicitly ask ssh_manager to refresh its info.

@yarikoptic, do you have a preference or see another, better option?

Another option (currently my preference):

Adjust get_connection to look up the identity file with datalad.config.get (no .reload before). Document that if python callers have modified the config/environment after loading datalad and want to ensure that things are up to date, they should call datalad.config.reload(force=True).

... if any external python callers want to set the environment variable...

"external" -- aren't you are using datalad Python API in reproman? shouldn't it then just change config directly (from datalad import cfg)? Then assure_initialized could memoize on either it was initialized with the current setting of the datalad.ssh.identityfile, and if value changes - reinitialize. Then you could even jump between different identities within the same process. (that is the "solution" I had in mind when I was leaving)

"external" -- aren't you are using datalad Python API in reproman?

Yes.

shouldn't it then just change config directly (from datalad import cfg)?

Regardless of what my specific code does, I was under the assumption that we wanted to support Python callers setting environment variables. That's the reason for the whole reload(force=True) discussion in the first place, but that was probably a silly assumption on my part. I guess if that were the case, we'd need cfg.reload(force=True) sprinkled all over the codebase.

So I don't mind going in the direction you suggest, though I'd prefer the last option I suggested, which would accomplish the same thing without the need for SSHManager to have an _identity_file attribute.

whatever you like the best. I do not think though it is really bad to store _identity_file attribute to provide more of automated handling, but may be I do not see possible side-effects (e.g. if we initiate some connection with out the identity_file, then need to initiate another with the identityfile -- would initialization kill existing sockets/connections? didn't check the code)

I do not think though it is really bad to store _identity_file attribute to provide more of automated handling

I'm failing to see what the extra automation is. Either way you have a config.get call. By doing it in get_connection you do it closer to the only place the value is currently used, don't need the extra code for updating the attribute, and avoid changing the way the current assure_initialized works.

Regarding your memory about needing it in environment variable... We might indeed need that (possibly to set it if not yet set while "treating config variable") because if we have a datalad special remote initiated by some annex call, it might be needed. But I don't think we have a specific use case for that yet I think.